fix: stop stratum TCP connection leak under reconnect load by iho · Pull Request #3889 · mimblewimble/grin

iho · 2026-07-09T07:34:39Z

Summary

Fixes #3867 / reports of stratum leaving thousands of TCP sockets open until the process exits (Too many open files, Windows loopback connection storms).

Root causes addressed

Sessions never timed out — half-open / abandoned peers could sit forever with no TCP keepalive and no app idle limit.
Unbounded write queue — futures unbounded mpsc + job broadcast kept feeding dead peers without backpressure.
No concurrent worker cap — reconnect storms could open unlimited FDs.
Cleanup was easy to skip — select + best-effort remove_worker (and expect panics) were fragile; worker_stats also grew forever.

Changes

Change	Why
`handle_connection` + `Drop` guard	Always `remove_worker` when the task ends
5-minute idle timeout	Close silent / half-open sessions
Max 256 workers	Hard FD / resource bound
Tokio bounded channel (64)	Backpressure; drop full/slow peers on broadcast
30s write timeout	Don't pin tasks on stalled sockets
`LinesCodec` max 64 KiB	Avoid huge-line memory abuse
Worker stats slot reuse	Reconnect storms don't grow the stats vec forever
Idempotent `remove_worker`	Safe double-cleanup

Tests

test_worker_slot_reuse_after_disconnect
test_try_send_to_missing_full_and_ok
test_remove_worker_is_idempotent
Existing RPC serde tests

cargo test -p grin_servers --lib mining::stratumserver -- --test-threads=1

Test plan

Unit tests above (11 passed)
Run a node with stratum enabled and a reconnecting miner; confirm worker count and FDs stay bounded
Confirm idle miners are disconnected after ~5 minutes without traffic
CI

Rework stratum accept/session handling so abandoned clients cannot pin file descriptors indefinitely: - Always free the worker slot via Drop guard on disconnect - Idle timeout (5m) for silent/half-open sessions - Cap concurrent workers (256) and reject excess accepts - Bounded per-worker write queue; drop slow/full peers - Write timeout; max RPC line length - Reuse disconnected worker_stats slots instead of growing forever - Idempotent remove_worker (no panic on double cleanup) Addresses mimblewimble#3867.

iho · 2026-07-09T07:36:28Z

Note: overlap with #3876

Both this PR and #3876 address #3867 with the same structural fix:

drop async_stream accept path
per-connection handle_connection + tokio::select!
RAII cleanup so workers are always removed on disconnect

#3876 is a smaller, cleaner refactor of that path (still uses unbounded queues; no idle timeout / worker cap).

This PR builds on the same idea and also bounds resources that keep half-open / abandoned sessions alive under reconnect load:

5m idle timeout
max 256 workers
bounded write queue + drop slow peers
write timeout, max RPC line length
worker_stats slot reuse + idempotent remove_worker

Happy to adjust the numeric limits if maintainers prefer different defaults. Also removed a stray // temp line that had snuck into the branch tip.

Add integration-style tests that spin a real accept loop on an ephemeral port, simulate reconnecting miners, assert worker count and FD growth stay bounded, and verify the max concurrent worker cap rejects surplus clients.

iho · 2026-07-09T07:40:04Z

Live reconnect / FD tests (now automated)

I initially only ran unit tests; the “run a node + reconnecting miner” items were manual and not executed yet. That was a gap.

They are now covered by automated tests in-tree:

cargo test -p grin_servers --lib mining::stratumserver::tests::test_live -- --test-threads=1

test_live_reconnect_storm_workers_and_fds_bounded — 150 connect/login/drop cycles + 40 concurrent holds; asserts workers drain to 0, FD count does not track reconnects, worker_stats slots are reused
test_live_max_workers_cap — holds MAX_STRATUM_WORKERS + 16 connections; asserts count ≤ 256 then drains to 0

Both passed locally (2 passed in ~1s after compile).

Still not a full mainnet node + grin-miner run, but it exercises the real accept/handle_connection path under reconnect load.

Script boots grin --usernet with stratum enabled, drives reconnecting TCP "miners", and asserts process FD count and established stratum sockets stay bounded after clients disconnect (issue mimblewimble#3867).

iho · 2026-07-09T07:46:10Z

Full-node soak now run

./scripts/stratum_reconnect_soak.sh

Result: PASS (local)

Metric	Value
Mode	`grin --usernet --no-tui` with stratum enabled, burn_reward, no seeds
Cycles	120 connect/login/drop
Concurrent	32 held then released
FD before → after	40 → 40 (delta 0)
ESTABLISHED stratum TCP after close	0
panics / EMFILE	none

This exercises the real binary accept path (not only the unit-test harness). Script lives at scripts/stratum_reconnect_soak.sh.

Pre-fill WorkersList to MAX_STRATUM_WORKERS with dummy channels instead of opening 256+ concurrent TCP sockets, which was unreliable under load (only ~117 workers registered). Still asserts surplus TCP accepts do not push the count past the cap.

Parameterize handle_connection idle timeout and add a live test that connects, stays silent past a short timeout, and asserts the worker is removed and the TCP session is closed. Production still uses the 5-minute WORKER_IDLE_TIMEOUT constant.

iho · 2026-07-09T07:50:32Z

Idle miner disconnect confirmed

Added test_live_idle_miner_disconnected:

Uses the same handle_connection idle path as production
Production constant remains WORKER_IDLE_TIMEOUT = 5 minutes
Test injects an 800ms idle timeout so CI does not wait 5 wall-clock minutes
Asserts: worker registers after login → still present mid-idle → count 0 after timeout → peer sees EOF/error

cargo test -p grin_servers --lib mining::stratumserver::tests::test_live_idle_miner_disconnected -- --nocapture
# ok

Full suite: 14 passed.

wiesche89

Thanks for addressing the leak. Since #3876 already provides the same structural fix in a smaller, more focused change, I think it would be the better base. We could add the relevant Rust regression tests there and handle the resource limits separately, as the current combined approach introduces new concurrency issues. The one off soak script should also stay out of the repository.

wiesche89 · 2026-07-11T11:27:41Z

+			match listener.accept().await {
+				Ok((socket, peer_addr)) => {
+					// Hard cap concurrent workers to avoid FD exhaustion.
+					if handler.workers.count() >= MAX_STRATUM_WORKERS {


This check runs before the spawned task registers the worker, so a burst of accepts can exceed the limit. Could the cap be enforced atomically inside add_worker?

wiesche89 · 2026-07-11T11:28:32Z

+		}
+		for id in slow {
+			warn!("Stratum: dropping slow/disconnected worker {}", id);
+			self.remove_worker(id);


Since worker IDs are reused, the old connection guard could remove a new worker that inherited the same ID. Could removal verify a generation token, or be left to the connection task?

wiesche89 · 2026-07-11T11:30:22Z

-			.expect("Stratum: no such addr in map");
+		if workers_list.remove(&worker_id).is_none() {
+			// Already removed (e.g. concurrent cleanup); still refresh counts.
+			let mut stratum_stats = self.stratum_stats.write();


add_worker takes stratum_stats before workers_list, but this branch takes them in the opposite order. Could we release workers_list before locking stratum_stats to avoid a possible deadlock?

wiesche89 · 2026-07-11T11:33:25Z

+					Some(line) => {
+						idle_deadline = Instant::now() + idle_timeout;
+						// Bound write time so a stalled peer cannot pin the task forever.
+						match timeout(Duration::from_secs(30), writer.send(line)).await {


could we make these 30 seconds a named constant as well, so all connection limits live in one place?

wiesche89 · 2026-07-11T11:34:12Z

+
+	loop {
+		tokio::select! {
+			biased;


Is the read first bias intentional? A continuously readable client can starve the writer until its response queue fills and the connection is dropped.

wiesche89 · 2026-07-11T11:39:29Z

@@ -0,0 +1,306 @@
+#!/usr/bin/env bash


Please remove this script from the PR. The relevant reconnect, FD cleanup, worker cap, and idle behavior is already covered by the automated Rust tests.

iho added 2 commits July 9, 2026 10:34

chore: remove accidental trailing comment from stratumserver tests

864dc7c

iho mentioned this pull request Jul 9, 2026

Refactor stratum server leak handling #3876

Open

test: live stratum reconnect storm and max-worker cap

ec1d494

Add integration-style tests that spin a real accept loop on an ephemeral port, simulate reconnecting miners, assert worker count and FD growth stay bounded, and verify the max concurrent worker cap rejects surplus clients.

test: add full-node stratum reconnect soak script

6b5f472

Script boots grin --usernet with stratum enabled, drives reconnecting TCP "miners", and asserts process FD count and established stratum sockets stay bounded after clients disconnect (issue mimblewimble#3867).

iho added 2 commits July 9, 2026 10:48

wiesche89 self-requested a review July 11, 2026 10:27

wiesche89 reviewed Jul 11, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: stop stratum TCP connection leak under reconnect load#3889

fix: stop stratum TCP connection leak under reconnect load#3889
iho wants to merge 6 commits into
mimblewimble:stagingfrom
iho:fix/stratum-connection-leak

iho commented Jul 9, 2026

Uh oh!

iho commented Jul 9, 2026

Uh oh!

iho commented Jul 9, 2026

Uh oh!

iho commented Jul 9, 2026

Uh oh!

iho commented Jul 9, 2026

Uh oh!

wiesche89 left a comment

Uh oh!

wiesche89 Jul 11, 2026

Uh oh!

wiesche89 Jul 11, 2026

Uh oh!

wiesche89 Jul 11, 2026

Uh oh!

wiesche89 Jul 11, 2026

Uh oh!

wiesche89 Jul 11, 2026

Uh oh!

wiesche89 Jul 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

iho commented Jul 9, 2026

Summary

Root causes addressed

Changes

Tests

Test plan

Uh oh!

iho commented Jul 9, 2026

Note: overlap with #3876

Uh oh!

iho commented Jul 9, 2026

Live reconnect / FD tests (now automated)

Uh oh!

iho commented Jul 9, 2026

Full-node soak now run

Uh oh!

iho commented Jul 9, 2026

Idle miner disconnect confirmed

Uh oh!

wiesche89 left a comment

Choose a reason for hiding this comment

Uh oh!

wiesche89 Jul 11, 2026

Choose a reason for hiding this comment

Uh oh!

wiesche89 Jul 11, 2026

Choose a reason for hiding this comment

Uh oh!

wiesche89 Jul 11, 2026

Choose a reason for hiding this comment

Uh oh!

wiesche89 Jul 11, 2026

Choose a reason for hiding this comment

Uh oh!

wiesche89 Jul 11, 2026

Choose a reason for hiding this comment

Uh oh!

wiesche89 Jul 11, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants